Skip to content

Fix/mwa keep connection alive auth token restore#269

Open
JoshhSandhu wants to merge 26 commits intomagicblock-labs:mainfrom
JoshhSandhu:fix/mwa-keepConnectionAlive-auth-token-restore
Open

Fix/mwa keep connection alive auth token restore#269
JoshhSandhu wants to merge 26 commits intomagicblock-labs:mainfrom
JoshhSandhu:fix/mwa-keepConnectionAlive-auth-token-restore

Conversation

@JoshhSandhu
Copy link
Copy Markdown
Contributor

@JoshhSandhu JoshhSandhu commented Mar 28, 2026

⚠️ This PR does not modify LocalAssociationScenario.cs. New
LocalAssociationScenario usages are marked with TODO comments to
switch to using var after PR #260 merges (which adds IDisposable).
This PR is compatible with PR #260 and will require a trivial rebase
on lines 63, 202, and 251 (var to using var) after #260 merges.

Status Type ⚠️ Core Change Issue
Ready Bug Yes #267

Problem

While building Unity Android games on Solana (CrossyRoad and SolRacer),
I found that after an explicit logout, the wallet silently reconnected
on the next app launch without showing an authorization prompt. The
player tapped Logout but the game behaved as if they never did.

Debugging traced this to three gaps in SolanaMobileWalletAdapter:

  1. Logout() never set _authToken = null. Within the same session,
    signing operations after logout would attempt Reauthorize() with
    a token the user intended to revoke.

  2. keepConnectionAlive = true restored Account from PlayerPrefs["pk"]
    without verifying the auth session. On process restart, _authToken
    was null, so the first SignTransaction() call triggered a full
    Authorize() popup during gameplay instead of at login time.

  3. IAdapterOperations had no Deauthorize() method. The wallet app was
    never notified of logout. Auth tokens remained valid indefinitely.

Solution

Lifecycle fixes (closes #267):

  • Added _authToken = null and PlayerPrefs.DeleteKey("authToken") to Logout(). base.Logout() is called first, then auth state is cleared
  • Persist authToken alongside pk on successful Authorize()
  • In _Login(), attempt Reauthorize() with cached token before returning
    cached Account. Fall through to full Authorize() if token is invalid
    or missing.
  • Restore _authToken from PlayerPrefs at top of _SignAllTransactions()
    and SignMessage() to handle cold start correctly

New RPC operations (MWA spec):

  • Deauthorize(string authToken): sends deauthorize JSON-RPC to wallet
  • GetCapabilities(): queries wallet feature limits, returns
    CapabilitiesResult with MaxTransactionsPerRequest,
    MaxMessagesPerRequest, SupportedTransactionVersions,
    SupportsCloneAuthorization

New lifecycle API:

  • DisconnectWallet(): reads cached token with PlayerPrefs fallback
    for cold start, sends Deauthorize RPC, clears all auth state.
    Deauthorize is best-effort, local cleanup always runs.
  • ReconnectWallet(): attempts silent reauthorize with cached token,
    fires OnWalletReconnected on success
  • OnWalletDisconnected and OnWalletReconnected events on both
    SolanaMobileWalletAdapter and SolanaWalletAdapter
  • GetCapabilities() passthrough on SolanaWalletAdapter

Before & After Screenshots

BEFORE -> wallet silently reconnects after explicit logout:

Open app
Connect wallet -- Seeker authorization prompt appears -- Approve
Close app
Reopen app -- No prompt -- wallet silently reconnected
Logout -- token never actually revoked on wallet side

AFTER -> auth cache correctly invalidated on logout:

AfterChanges.1.mp4

Open app
Connect wallet -- Seeker authorization prompt appears -- Approve
Close app without logout
Reopen app -- Silent reauthorize -- Connected instantly -- No popup
Logout -- Deauthorize RPC sent to Seeker -- cache cleared
Reopen app -- Seeker authorization prompt appears -- Fresh approval required

Tested on Android (Seeker wallet via Mobile Wallet Adapter) in:

Other changes

  • CapabilitiesResult.cs added alongside existing response models
    following same Preserve and JsonProperty conventions
  • Events fire after state is fully updated, not before
  • DisconnectWallet() has try/catch so transport failure never blocks
    local logout
  • All new changes are backward compatible, no existing constructor
    signatures or public method signatures changed

Summary by CodeRabbit

  • New Features

    • Wallet disconnect/reconnect actions with new events for connection-state changes.
    • Queryable wallet capabilities: clone-authorization support, transaction/message limits, supported transaction versions.
    • Explicit deauthorization operation to revoke stored session tokens.
  • Chores / Behavior

    • Session persistence and signing flows improved: lazy-load/reuse of stored auth tokens, reauthorization flow, persist updated tokens, and explicit clearing of tokens on logout or failed reauth.

_authToken was never set to null on Logout(), allowing reauthorize
attempts with a revoked token within the same session. authToken
PlayerPrefs key was also never cleared, leaving a stale token
on disk after logout.

Fixes part of magicblock-labs#267
…ve login

When keepConnectionAlive = true, _Login() previously restored Account
from PlayerPrefs["pk"] without verifying authorization state this caused
_authToken to be null on process restart, triggering an unexpected
Authorize() popup mid-transaction instead of at login time.

Fix: attempt Reauthorize() with cached authToken before returning cached
Account. On failure, clear both cached keys and fall through to full
Authorize() flow, also persist authToken alongside pk on successful
Authorize so the token survives process restart.

Fixes magicblock-labs#267
Added two missing MWA spec operations to the adapter interface:

- Deauthorize(string authToken): revokes wallet-side auth token.
  Takes authToken only -- no identity fields per MWA spec.
  Returns non-generic Task (no payload on success).

- GetCapabilities(): queries wallet feature limits and supported
  transaction versions. Returns new CapabilitiesResult model with
  MaxTransactionsPerRequest, MaxMessagesPerRequest,
  SupportedTransactionVersions, and SupportsCloneAuthorization.

CapabilitiesResult.cs added alongside existing response models
following same Preserve and JsonProperty conventions.

Related to magicblock-labs#267
Added Deauthorize(string authToken) and PrepareDeauthorizeRequest()
to MobileWalletAdapterClient.

The deauthorize RPC sends only auth_token on the wire, no identity
fields, matching the MWA spec, result is discarded as the RPC
returns no payload on success.

Related to magicblock-labs#267
Added GetCapabilities() and PrepareGetCapabilitiesRequest() to
MobileWalletAdapterClient.

Queries wallet feature limits via get_capabilities RPC. Takes no
parameters per MWA spec. Returns CapabilitiesResult with
MaxTransactionsPerRequest, MaxMessagesPerRequest,
SupportedTransactionVersions, and SupportsCloneAuthorization.

Related to magicblock-labs#267
…vents

Added OnWalletDisconnected and OnWalletReconnected events and two new
public async methods to SolanaMobileWalletAdapter:

DisconnectWallet(): captures auth token from memory with PlayerPrefs
fallback, sends deauthorize RPC, then calls Logout() to clear all
local state event fires after cleanup deauthorize failure is logged
but never blocks local logout.

ReconnectWallet(): attempts silent reconnect via Login() which uses
the cached token reauthorize path event fires after Login() succeeds.
Errors are logged without re-throwing.

Related to magicblock-labs#267
…to SolanaWalletAdapter

Exposed DisconnectWallet() and ReconnectWallet() on the cross-platform
SolanaWalletAdapter wrapper. Both methods cast _internalWallet to
SolanaMobileWalletAdapter and delegate. NotImplementedException thrown
for non-mobile adapter targets.

Related to magicblock-labs#267
After process restart, _authToken is null in memory even when a valid
token exists in PlayerPrefs from a successful reauthorize. This caused
_SignAllTransactions() and SignMessage() to fall into the Authorize()
branch unnecessarily, triggering an unexpected wallet popup mid-session.

Fix: read authToken from PlayerPrefs at the top of both signing methods
before the Authorize/Reauthorize branch is evaluated.

Fixes magicblock-labs#267
LocalAssociationScenario does not implement IDisposable in the current
codebase. using var causes a compile error against main.

Changed to plain var with TODO comments on lines 63 and 196.
Will be updated to using var after PR magicblock-labs#260 merges, which adds
IDisposable to LocalAssociationScenario.

Related to magicblock-labs#267
…e path

Two fixes in the keepConnectionAlive reauthorize guard:

1. Added null check on reauth result before assigning to _authToken.
   A null reauth or empty AuthToken would silently set _authToken to
   null, re-introducing the mid-transaction Authorize() popup from
   Issue magicblock-labs#267.

2. Persist the (possibly rotated) token back to PlayerPrefs after a
   successful reauthorize. Wallets that rotate auth tokens on every
   reauthorize call would otherwise force a full Authorize() popup
   on every cold start despite keepConnectionAlive = true.

Fixes magicblock-labs#267
M-1: wrap DisconnectWallet() StartAndExecute in try/catch so
Logout() and OnWalletDisconnected always fire even on transport
exceptions. Deauthorize is best-effort, local cleanup is guaranteed.

M-2: guard OnWalletReconnected behind null check on Login() return.
Prevents false-positive reconnect event when Login() returns null.

M-4: add GetCapabilities() to SolanaMobileWalletAdapter and forward
through SolanaWalletAdapter using same cast-and-delegate pattern.

M-5: add OnWalletDisconnected and OnWalletReconnected events to
SolanaWalletAdapter. Constructor wires them to mobile adapter events
under UNITY_ANDROID guard.

Related to magicblock-labs#267
…ilities() parameters

- Added a warning log for failed Deauthorize() RPC responses to improve error visibility.
- Changed Params in GetCapabilities() to use a new JsonRequest.JsonRequestParams() object for better clarity and adherence to expected structure.
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 28, 2026

Warning

Rate limit exceeded

@JoshhSandhu has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 43 minutes and 38 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 43 minutes and 38 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 77f18569-1306-433e-ab2d-b06d32ebb4bb

📥 Commits

Reviewing files that changed from the base of the PR and between f2f08e5 and dfefec2.

📒 Files selected for processing (1)
  • Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs

Walkthrough

Adds Deauthorize and GetCapabilities RPCs and a CapabilitiesResult DTO; persists and restores auth tokens via PlayerPrefs; introduces Disconnect/Reconnect APIs and events; updates login, logout, and signing flows to prefer reauthorize, persist returned auth tokens, and clear tokens on logout/failure.

Changes

Cohort / File(s) Summary
Interface
Runtime/codebase/SolanaMobileStack/Interfaces/IAdapterOperations.cs
Added Task Deauthorize(string authToken) and Task<CapabilitiesResult> GetCapabilities() to the public adapter operations interface.
Response model & meta
Runtime/codebase/SolanaMobileStack/JsonRpcClient/Responses/CapabilitiesResult.cs, Runtime/codebase/SolanaMobileStack/JsonRpcClient/Responses/CapabilitiesResult.cs.meta
Added CapabilitiesResult DTO (nullable capability flags/limits and SupportedTransactionVersions[]) and Unity metadata file.
RPC client
Runtime/codebase/SolanaMobileStack/MobileWalletAdapterClient.cs
Added Deauthorize(string authToken) and GetCapabilities() RPC methods with request builders; introduced shared JsonRpcVersion constant and updated request prep to use it.
Mobile wallet adapter (logic & lifecycle)
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
Persist/load _authToken via PlayerPrefs; require cached auth token for silent reauthorization; clear persisted token on logout/failure; tightened null-checks; persist returned auth tokens when appropriate; added DisconnectWallet(), ReconnectWallet(), GetCapabilities() and events OnWalletDisconnected/OnWalletReconnected.
Platform-facing adapter
Runtime/codebase/SolanaWalletAdapter.cs
Exposed OnWalletDisconnected/OnWalletReconnected events and DisconnectWallet(), ReconnectWallet(), GetCapabilities() methods; on Android forwards to internal SolanaMobileWalletAdapter, otherwise throws or no-ops/returns null as appropriate.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 3.45% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Fix/mwa keep connection alive auth token restore' accurately summarizes the main change: fixing the auth token restoration issue in the keep connection alive feature.
Description check ✅ Passed The PR description comprehensively covers problem, solution, testing, and before/after scenarios, closely following the provided template structure with all key sections populated.
Linked Issues check ✅ Passed All coding requirements from issue #267 are met: Logout() clears auth state, authToken is persisted and restored, Deauthorize() RPC is implemented, Reauthorize() validates sessions on restart, and lifecycle APIs (DisconnectWallet, ReconnectWallet, events) are provided.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing the auth token lifecycle issue from #267; changes include interface updates, new RPC operations, lifecycle APIs, and state management improvements, all within the stated scope.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Runtime/codebase/SolanaMobileStack/Interfaces/IAdapterOperations.cs`:
- Around line 15-19: Add the explicit public modifier to the interface members
in IAdapterOperations for consistency with existing members: change the
Deauthorize and GetCapabilities declarations to include the public keyword
(i.e., make the signatures public Task Deauthorize(string authToken); and public
Task<CapabilitiesResult> GetCapabilities();) so they match the style of other
methods in the IAdapterOperations interface.

In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 228-246: ReconnectWallet currently swallows exceptions (logs
e.Message) so callers can't tell failure from success; update ReconnectWallet to
rethrow the caught exception after logging (use throw;) so callers of
ReconnectWallet can handle failures, and ensure any callers of ReconnectWallet
handle the propagated exception; reference: ReconnectWallet, Login,
OnWalletReconnected.
- Around line 63-93: The async lambda passed to StartAndExecute uses
Action<IAdapterOperations> which makes it effectively async void and can swallow
exceptions; change the delegate type to Func<IAdapterOperations, Task> (update
LocalAssociationScenario.StartAndExecute and ExecuteNextAction to accept and
await IEnumerable<Func<IAdapterOperations, Task>>) and replace
Action<IAdapterOperations> with Func<IAdapterOperations, Task> where you build
the reauthorizeScenario actions so the async client => { ... } returns a Task,
ensuring exceptions from Reauthorize propagate and are awaited before checking
reauthorizeResult.WasSuccessful and updating _authToken.

In `@Runtime/codebase/SolanaWalletAdapter.cs`:
- Around line 112-120: Add an XML doc comment to the GetCapabilities method that
documents the possible null return and when it occurs: explain that
GetCapabilities returns a CapabilitiesResult for SolanaMobileWalletAdapter,
throws NotImplementedException when _internalWallet is non-null but unsupported,
and returns null when _internalWallet is null or when running on platforms that
don't support mobile adapters; reference GetCapabilities, _internalWallet,
SolanaMobileWalletAdapter, and CapabilitiesResult in the summary/remarks so
callers know to handle the null case.
- Around line 88-110: Both DisconnectWallet and ReconnectWallet currently
silently return when _internalWallet is null, causing inconsistent behavior with
other methods; update both methods (DisconnectWallet and ReconnectWallet) to
handle the null case consistently by throwing a NotImplementedException (or at
minimum logging a warning) when _internalWallet is null, while retaining the
existing cast to SolanaMobileWalletAdapter and the await/return flow when the
cast succeeds so behavior matches _Login and other adapter methods.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 70b132c7-d3b8-4dbf-9bca-b5ca330a04f2

📥 Commits

Reviewing files that changed from the base of the PR and between 054503e and 8ef67ae.

📒 Files selected for processing (6)
  • Runtime/codebase/SolanaMobileStack/Interfaces/IAdapterOperations.cs
  • Runtime/codebase/SolanaMobileStack/JsonRpcClient/Responses/CapabilitiesResult.cs
  • Runtime/codebase/SolanaMobileStack/JsonRpcClient/Responses/CapabilitiesResult.cs.meta
  • Runtime/codebase/SolanaMobileStack/MobileWalletAdapterClient.cs
  • Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
  • Runtime/codebase/SolanaWalletAdapter.cs

Comment thread Runtime/codebase/SolanaMobileStack/Interfaces/IAdapterOperations.cs
Comment thread Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
Comment thread Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
Comment thread Runtime/codebase/SolanaWalletAdapter.cs
Comment thread Runtime/codebase/SolanaWalletAdapter.cs
- Add public modifier to Deauthorize and GetCapabilities in IAdapterOperations
- Rethrow exception in ReconnectWallet so callers can detect failure
- Add comment for null wallet case in DisconnectWallet and ReconnectWallet
- Add XML doc comment to GetCapabilities explaining null return case
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (2)
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs (1)

250-270: 🧹 Nitpick | 🔵 Trivial

GetCapabilities may return null on silent lambda failure.

Due to the async void lambda pattern, if client.GetCapabilities() throws an exception, it will be swallowed, result.WasSuccessful may still be true (depending on how the scenario handles async exceptions), and capabilities will remain null. Callers should be aware of this edge case.

This is part of the broader architectural concern deferred to PR #260 per retrieved learnings, so no immediate action is required. However, consider adding a null check with a warning before returning:

♻️ Optional defensive check
         if (!result.WasSuccessful)
         {
             Debug.LogError(result.Error.Message);
             throw new Exception(result.Error.Message);
         }
+        if (capabilities == null)
+        {
+            Debug.LogWarning("[MWA] GetCapabilities succeeded but returned null");
+        }
         return capabilities;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs` around lines
250 - 270, GetCapabilities can return null if the async lambda passed to
LocalAssociationScenario.StartAndExecute swallows exceptions (e.g.,
client.GetCapabilities() fails); add a defensive null-check after the scenario
completes: if capabilities is null, log a descriptive error via Debug.LogError
and throw an exception (or return a failed CapabilitiesResult) so callers never
receive null. Locate the GetCapabilities method, the
localAssociationScenario/StartAndExecute call and the client.GetCapabilities()
invocation to implement the check and error handling.
Runtime/codebase/SolanaWalletAdapter.cs (1)

101-111: 🧹 Nitpick | 🔵 Trivial

ReconnectWallet silently returns when no wallet is configured.

Unlike DisconnectWallet which has an explanatory comment, ReconnectWallet silently returns when _internalWallet is null. For a reconnect operation, callers might reasonably expect an exception or a return value indicating failure.

Consider either adding a comment explaining the silent return, or throwing to be consistent with _Login:

♻️ Option 1: Add explanatory comment
         if (_internalWallet != null)
             throw new NotImplementedException();
+        // No internal wallet configured - nothing to reconnect
     }
♻️ Option 2: Throw for consistency with _Login
         if (_internalWallet != null)
             throw new NotImplementedException();
+        throw new InvalidOperationException("No wallet configured to reconnect");
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Runtime/codebase/SolanaWalletAdapter.cs` around lines 101 - 111, The
ReconnectWallet method currently silently returns when _internalWallet is null;
change it to be explicit like _Login by throwing an InvalidOperationException
(or a more specific exception) when _internalWallet is null, and include a brief
message such as "No wallet configured to reconnect" to aid callers; update the
method handling in ReconnectWallet (and optionally add a short comment above it
mirroring DisconnectWallet's style if you prefer the explanatory-comment
approach) so callers receive a clear failure instead of a silent return.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 250-270: GetCapabilities can return null if the async lambda
passed to LocalAssociationScenario.StartAndExecute swallows exceptions (e.g.,
client.GetCapabilities() fails); add a defensive null-check after the scenario
completes: if capabilities is null, log a descriptive error via Debug.LogError
and throw an exception (or return a failed CapabilitiesResult) so callers never
receive null. Locate the GetCapabilities method, the
localAssociationScenario/StartAndExecute call and the client.GetCapabilities()
invocation to implement the check and error handling.

In `@Runtime/codebase/SolanaWalletAdapter.cs`:
- Around line 101-111: The ReconnectWallet method currently silently returns
when _internalWallet is null; change it to be explicit like _Login by throwing
an InvalidOperationException (or a more specific exception) when _internalWallet
is null, and include a brief message such as "No wallet configured to reconnect"
to aid callers; update the method handling in ReconnectWallet (and optionally
add a short comment above it mirroring DisconnectWallet's style if you prefer
the explanatory-comment approach) so callers receive a clear failure instead of
a silent return.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 66ea8686-94c9-408a-a3c5-9f7fb360bcfe

📥 Commits

Reviewing files that changed from the base of the PR and between 8ef67ae and a0cefb2.

📒 Files selected for processing (3)
  • Runtime/codebase/SolanaMobileStack/Interfaces/IAdapterOperations.cs
  • Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
  • Runtime/codebase/SolanaWalletAdapter.cs

- Add null check warning in GetCapabilities when result is null
- Add explanatory comment in ReconnectWallet null wallet case
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs (1)

278-310: ⚠️ Potential issue | 🔴 Critical

SignMessage() can dereference Account after a cold start.

With the new token restore, this method can now run before Login(). In that path Account is still null, so Line 309 can throw before the request is sent. Use the public key returned in authorization (or the cached pk) when the in-memory Account is unavailable.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs` around lines
278 - 310, SignMessage() can dereference Account after a cold start; modify the
SignMessages call in the SignMessage method so it uses the public key from the
authorization result (authorization.PublicKey) or a cached public-key fallback
instead of directly using Account.PublicKey.KeyBytes when Account is null.
Concretely: inside the async client => { signedMessages = await
client.SignMessages(...) } block, compute the address bytes as
Account?.PublicKey?.KeyBytes ?? authorization?.PublicKey?.KeyBytes ??
cachedPublicKeyBytes (or similar cached pk field) and pass that into the
addresses list so the call never dereferences a null Account.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 140-141: The code restores _authToken from PlayerPrefs
unconditionally and only updates the in-memory _authToken after successful
Authorize()/Reauthorize(), which breaks the intended
_walletOptions.keepConnectionAlive behavior; change the restore so
PlayerPrefs.GetString("authToken") is only used when
_walletOptions.keepConnectionAlive is true (gate the restore behind that flag),
and after successful Authorize() and Reauthorize() persist the refreshed auth
token and PublicKey back to PlayerPrefs (PlayerPrefs.SetString("authToken", ...)
and SetString("publicKey", ...) or remove them when keepConnectionAlive is
false) so both memory and persistent state remain consistent. Apply the same
gating and persistence fixes where _authToken/PublicKey are read or written
(refer to usages around the existing _authToken read, and the Authorize(),
Reauthorize() methods and the code regions you flagged at 178-179, 278-279,
319-320).
- Around line 60-88: The code currently constructs Account using the cached pk
even after a successful Reauthorize; instead, use the Reauthorize result's
public key (reauth.PublicKey) as the source of truth: when
reauthorizeResult.WasSuccessful and reauth is non-null and reauth.PublicKey is
not empty, persist that public key back to the same PlayerPrefs key you
originally read pk from and construct/return new Account using reauth.PublicKey
(rather than pk); update any local variables that represent the cached key so
subsequent logic uses reauth.PublicKey.

---

Outside diff comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 278-310: SignMessage() can dereference Account after a cold start;
modify the SignMessages call in the SignMessage method so it uses the public key
from the authorization result (authorization.PublicKey) or a cached public-key
fallback instead of directly using Account.PublicKey.KeyBytes when Account is
null. Concretely: inside the async client => { signedMessages = await
client.SignMessages(...) } block, compute the address bytes as
Account?.PublicKey?.KeyBytes ?? authorization?.PublicKey?.KeyBytes ??
cachedPublicKeyBytes (or similar cached pk field) and pass that into the
addresses list so the call never dereferences a null Account.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: fac9c4e3-a6c7-4b9d-b820-c687ec938948

📥 Commits

Reviewing files that changed from the base of the PR and between a0cefb2 and 05273e3.

📒 Files selected for processing (2)
  • Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
  • Runtime/codebase/SolanaWalletAdapter.cs

Comment thread Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs Outdated
Comment thread Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs Outdated
- Use reauth.PublicKey as source of truth after successful reauthorize
  instead of cached PlayerPrefs pk. Falls back to cached pk only when
  wallet returns no key.
- Gate authToken PlayerPrefs restore and persist behind keepConnectionAlive
  flag in _SignAllTransactions() and SignMessage(). Persists rotated token
  after every successful signing operation.
- Guard Account.PublicKey.KeyBytes in SignMessage() against null Account
  on cold start using cached pk from PlayerPrefs as fallback. Throws
  clear exception if neither is available.
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 291-294: The code captures cachedPk from Account/PublicKey or
PlayerPrefs before the wallet authorization, which can mismatch the authorized
session; update the signing flow to use the authorized public key
(authorization.PublicKey) when present instead of the pre-captured cachedPk —
move or perform the signing logic inside the same callback/lambda where
authorization (from Authorize() or Reauthorize()) is assigned, or after
authorization completes, and fall back to Account?.PublicKey or PlayerPrefs only
if authorization.PublicKey is null; ensure you update references to cachedPk
used in the signer method to read authorization.PublicKey when available to keep
the signed message consistent with the active authorized account.
- Around line 279-283: In SolanaMobileWalletAdapter.GetCapabilities, instead of
logging a warning and returning null when capabilities is null after a
successful RPC (result.WasSuccessful == true), throw an exception (e.g.,
InvalidOperationException) with a clear message that the RPC completed but
returned no capabilities; include any available context (method name, maybe
result metadata) in the exception message so callers cannot mistakenly treat a
null as success and will be forced to handle the unexpected empty response.
- Around line 85-93: When reauthorizeResult.WasSuccessful is true you still must
verify that _authToken was actually populated; update the block handling
reauthorizeResult (check _authToken for null/empty after reauthorization) and
only persist PlayerPrefs and return the new Account (using resolvedKey from
reauthPublicKey or pk) if _authToken is non-empty. If _authToken is empty, treat
the reauthorization as failed (clear any stale PlayerPrefs authToken if present,
do not return the Account, and surface the failure path — e.g., return a
failure/null result or throw) so subsequent calls will invoke the proper
authorization flow instead of assuming success.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 50817458-72c4-4192-a71f-057694889658

📥 Commits

Reviewing files that changed from the base of the PR and between 05273e3 and b730145.

📒 Files selected for processing (1)
  • Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs

Comment thread Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs Outdated
Comment thread Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
Comment thread Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
- Treat reauthorize as failure when WasSuccessful but token is empty
  Clear cached keys and fall through to full Authorize flow
- Throw exception in GetCapabilities when RPC succeeds but returns
  null data instead of silently returning null to caller
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs (1)

336-346: ⚠️ Potential issue | 🟡 Minor

Potential null dereference if authorization is not populated.

Same issue as in _SignAllTransactions: if the first lambda's Authorize/Reauthorize call fails silently, authorization could remain null at line 341.

🛡️ Proposed defensive check
         if (!result.WasSuccessful)
         {
             Debug.LogError(result.Error.Message);
             throw new Exception(result.Error.Message);
         }
+        if (authorization == null)
+        {
+            throw new Exception("[MWA] SignMessage: authorization was not populated");
+        }
         _authToken = authorization.AuthToken;
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs` around lines
336 - 346, authorization may be null even when result.WasSuccessful is true, so
add a defensive null-check before reading authorization.AuthToken in the code
after the authorization flow (and mirror the same fix in _SignAllTransactions):
if authorization is null or authorization.AuthToken is null/empty, log a clear
error via Debug.LogError (include context like "authorization is null after
Authorize/Reauthorize") and throw a meaningful Exception instead of
dereferencing; otherwise assign _authToken and continue with the existing
PlayerPrefs logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 85-104: The reauthorization-success branch checks
reauthorizeResult.WasSuccessful and then deletes PlayerPrefs keys when
_authToken is empty, but execution then falls through to the final cleanup that
deletes the same keys again; modify the control flow in the block that checks
_authToken (inside the reauthorizeResult.WasSuccessful branch in
SolanaMobileWalletAdapter.cs) so cleanup happens only once—either by returning
immediately after performing the deletion for the empty-token case or by
restructuring the logic to have a single shared cleanup path after the
reauthorizeResult handling; reference the reauthorizeResult.WasSuccessful check,
the _authToken variable, the reauthPublicKey/pk resolution that returns new
Account(...), and PlayerPrefs.DeleteKey/PlayerPrefs.Save to locate and update
the code.
- Around line 190-196: The code assumes authorization is non-null when setting
_authToken and persisting it; add a defensive null check for the local variable
authorization before using it (e.g., inside the method that currently references
authorization, check if authorization == null), and if null either throw a clear
exception or return an appropriate empty result (e.g., an empty Transaction[]),
avoiding the NullReferenceException; ensure the logic that persists _authToken
(when _walletOptions.keepConnectionAlive) and the subsequent use of
res.SignedPayloads/Transaction.Deserialize only runs when authorization is
non-null.

---

Outside diff comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 336-346: authorization may be null even when result.WasSuccessful
is true, so add a defensive null-check before reading authorization.AuthToken in
the code after the authorization flow (and mirror the same fix in
_SignAllTransactions): if authorization is null or authorization.AuthToken is
null/empty, log a clear error via Debug.LogError (include context like
"authorization is null after Authorize/Reauthorize") and throw a meaningful
Exception instead of dereferencing; otherwise assign _authToken and continue
with the existing PlayerPrefs logic.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 723be0b8-31e1-421a-b7f1-a3bb366910b9

📥 Commits

Reviewing files that changed from the base of the PR and between b730145 and e3954d2.

📒 Files selected for processing (1)
  • Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs

Comment thread Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
Comment thread Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs Outdated
- Remove redundant PlayerPrefs deletion in empty-token reauthorize
  failure path, replace with fall-through comment
- Add null guard on authorization object after StartAndExecute in
  _Login(), _SignAllTransactions(), and SignMessage() to prevent
  silent NullReferenceException from async void lambda edge case
@JoshhSandhu
Copy link
Copy Markdown
Contributor Author

@coderabbitai resume

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 29, 2026

✅ Actions performed

Reviews resumed.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (2)
Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs (2)

94-97: ⚠️ Potential issue | 🟠 Major

Persist the reauthorized public key, not just the token.

This branch returns resolvedKey but only writes authToken back to disk. If the wallet reauthorizes a different account, PlayerPrefs["pk"] stays stale and a later cold-start SignMessage() can still target the old address. Save resolvedKey alongside the refreshed token, and mirror that refresh after successful auth in the signing methods.

Based on learnings: SignMessage() intentionally uses cachedPk from Account?.PublicKey or PlayerPrefs.GetString("pk") under the current Action<IAdapterOperations> pattern, so stale persisted keys remain on the active signing path.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs` around lines
94 - 97, The branch that handles reauthorization currently persists only the
refreshed _authToken via PlayerPrefs.SetString("authToken", _authToken) but
returns resolvedKey; update this block to also persist the resolvedKey (e.g.,
PlayerPrefs.SetString("pk", resolvedKey) and PlayerPrefs.Save()) so
PlayerPrefs["pk"] is not stale, and ensure after successful auth the in-memory
Account/PublicKey (and any cachedPk used by SignMessage) is updated to the new
PublicKey so signing methods (SignMessage and any code reading
Account?.PublicKey or PlayerPrefs.GetString("pk")) see the refreshed key
immediately.

197-202: ⚠️ Potential issue | 🟠 Major

Don't replace a valid session with an empty AuthToken.

_Login() already treats a successful Reauthorize() with no token as failure, but both signing paths still assign and persist authorization.AuthToken unconditionally. If the wallet reports success without a token here, the current sign works and the next request falls back to Authorize() again, reintroducing the mid-session prompt this PR is fixing. Only overwrite/persist the token when it is non-empty; otherwise keep the previous token or surface the failure explicitly.

Also applies to: 352-356

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs` around lines
197 - 202, The code is overwriting and persisting _authToken with
authorization.AuthToken even when that token is empty; change the signing-path
assignments (the block that sets _authToken and calls PlayerPrefs.SetString when
_walletOptions.keepConnectionAlive is true) to only update and persist the token
if authorization.AuthToken is not null/empty—leave the existing _authToken
unchanged (or surface the failure) when the returned AuthToken is empty; apply
the same conditional check to the other occurrence around the code referenced at
lines 352-356 so both signing flows follow the same non-empty-token guard and
avoid replacing a valid session with an empty token.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 139-141: The code is storing the wallet bearer token in
PlayerPrefs (PlayerPrefs.SetString("authToken", _authToken)), which is insecure;
replace use of PlayerPrefs for the auth token with platform-secure storage:
persist _authToken into an encrypted/keystore-backed storage (e.g., Android
Keystore / EncryptedSharedPreferences or an existing secure-storage
abstraction), and update load/clear logic used by Reauthorize() and
Deauthorize() to read/erase from that secure storage instead of PlayerPrefs
(leave publicKey in PlayerPrefs if acceptable). Ensure the new storage API
encrypts at rest and is used for Set/Get/Delete of the auth token across the
SolanaMobileWalletAdapter class.
- Around line 193-203: Guard the signing result before dereferencing: in
SignAllTransactions (and the similar SignTransaction(s) path around the 348-358
range) check that the local result object (res) and its payload collection
(res.SignedPayloads / signedMessages) are not null before calling .Select(...)
or Transaction.Deserialize; if they are null, handle gracefully (e.g. return an
empty Transaction[] or throw a clear exception) and ensure the failure is
propagated (set the operation result as failed) so swallowed exceptions in the
async Action<IAdapterOperations> callback can't lead to NullReferenceException.

---

Duplicate comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 94-97: The branch that handles reauthorization currently persists
only the refreshed _authToken via PlayerPrefs.SetString("authToken", _authToken)
but returns resolvedKey; update this block to also persist the resolvedKey
(e.g., PlayerPrefs.SetString("pk", resolvedKey) and PlayerPrefs.Save()) so
PlayerPrefs["pk"] is not stale, and ensure after successful auth the in-memory
Account/PublicKey (and any cachedPk used by SignMessage) is updated to the new
PublicKey so signing methods (SignMessage and any code reading
Account?.PublicKey or PlayerPrefs.GetString("pk")) see the refreshed key
immediately.
- Around line 197-202: The code is overwriting and persisting _authToken with
authorization.AuthToken even when that token is empty; change the signing-path
assignments (the block that sets _authToken and calls PlayerPrefs.SetString when
_walletOptions.keepConnectionAlive is true) to only update and persist the token
if authorization.AuthToken is not null/empty—leave the existing _authToken
unchanged (or surface the failure) when the returned AuthToken is empty; apply
the same conditional check to the other occurrence around the code referenced at
lines 352-356 so both signing flows follow the same non-empty-token guard and
avoid replacing a valid session with an empty token.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: cfe3add6-7df9-400a-9c61-3418bc483eb0

📥 Commits

Reviewing files that changed from the base of the PR and between b730145 and 175ad9c.

📒 Files selected for processing (1)
  • Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs

Comment thread Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs Outdated
Added null checks for res in _SignAllTransactions() and signedMessages
in SignMessage() to prevent NullReferenceException when the signing
callback fails silently due to the async void Action pattern.
@JoshhSandhu
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 29, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@JoshhSandhu
Copy link
Copy Markdown
Contributor Author

@Kuldotha CodeRabbit checks are resolved and this has been tested on an Android device inside a gameplay loop.

This fixes the keepConnectionAlive auth session lifecycle issue from #267 and should be ready for review whenever you have time.

Happy to adjust anything needed.

- Add PrefKeyPublicKey (solana_sdk.mwa.public_key) and PrefKeyAuthToken
  (solana_sdk.mwa.auth_token) constants to replace bare string literals
- Add JsonRpcVersion constant in MobileWalletAdapterClient
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Runtime/codebase/SolanaMobileStack/MobileWalletAdapterClient.cs`:
- Around line 50-60: Deauthorize currently uses async/await for a single
terminal SendRequest<object> call which is inconsistent with siblings and
introduces an unnecessary state machine; change the method signature to return
Task (remove async) and return SendRequest<object>(request) directly in
Deauthorize (referencing the Deauthorize method and SendRequest<object> call) so
it matches the pattern used by
Authorize/Reauthorize/SignTransactions/SignMessages/GetCapabilities and avoids
wrapping exceptions differently.

In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 196-210: SignAllTransactions unconditionally assigns and persists
authorization.AuthToken which can overwrite a valid cached _authToken with an
empty/null token; mirror the _Login behavior by checking authorization.AuthToken
for null/empty before setting _authToken and before calling
PlayerPrefs.SetString: in SignAllTransactions (and likewise in SignMessage) only
assign _authToken and persist to PlayerPrefs when authorization.AuthToken is
non-empty, otherwise clear cached credentials like _Login does (remove
PrefKeyAuthToken or avoid writing) and avoid silently invalidating the persisted
session.
- Around line 60-112: The code misses the case where an auth token is cached but
the public key is empty, leaving an orphan PrefKeyAuthToken in PlayerPrefs;
inside the keep-alive block (where _walletOptions.keepConnectionAlive is checked
and PlayerPrefs.GetString(PrefKeyPublicKey, null) and PrefKeyAuthToken are read)
add a branch to detect when pk is null/empty but authToken is not and delete
PrefKeyAuthToken (and Save) so cached credentials are cleaned symmetrically;
update the logic near the LocalAssociationScenario/reauthorize flow (references:
LocalAssociationScenario, StartAndExecute, PrefKeyPublicKey, PrefKeyAuthToken,
PlayerPrefs) to remove the orphan token in that case.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 0e7809ff-3408-4fb4-8e9c-4ff1d5e4e510

📥 Commits

Reviewing files that changed from the base of the PR and between ac90023 and ffb32de.

📒 Files selected for processing (2)
  • Runtime/codebase/SolanaMobileStack/MobileWalletAdapterClient.cs
  • Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs

Comment thread Runtime/codebase/SolanaMobileStack/MobileWalletAdapterClient.cs Outdated
Comment thread Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
Comment thread Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
removed unnecessary async/await from deauthorize and return the task directly to avoid an extra state machine/allocation

Guard setting and persisting _authToken so it's only assigned and stored when authorization.AuthToken is non-empty, and only persist when keepConnectionAlive is true to avoid overwriting valid tokens with empty values
@JoshhSandhu
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 16, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 284-308: GetCapabilities currently creates a new
LocalAssociationScenario and RPC round-trip every call; cache the
CapabilitiesResult per session/auth token to avoid redundant calls. Add a
private field (e.g., _cachedCapabilities and _cachedAuthToken) on the class and
in GetCapabilities return the cached value if the current _authToken equals
_cachedAuthToken; otherwise run the existing LocalAssociationScenario flow (the
code inside GetCapabilities and the async client => { capabilities = await
client.GetCapabilities(); }) and store the returned CapabilitiesResult and auth
token into the cache before returning. Ensure you clear or update the cache when
the auth token changes (or association ends) and consider minimal
synchronization (lock or Interlocked) around reads/writes to _cachedCapabilities
to handle concurrent callers.
- Around line 217-224: Update the XML documentation for the public Logout()
method to state clearly that Logout clears in-memory _authToken and persisted
PlayerPrefs keys (PrefKeyPublicKey, PrefKeyAuthToken) and calls base.Logout(),
but does NOT call Deauthorize() or revoke the wallet-side token; add guidance
that consumers who need wallet-side revocation should call DisconnectWallet()
(which performs Deauthorize()) instead.
- Around line 138-144: In _Login where you handle the fresh Authorize()
response, avoid unconditionally assigning and persisting an empty auth token:
only set _authToken and call PlayerPrefs.SetString(PrefKeyAuthToken, ...) when
authorization.AuthToken is non-empty; continue to persist the public key
(publicKey.ToString()) if _walletOptions.keepConnectionAlive is true, but wrap
the _authToken assignment and PrefKeyAuthToken write in the same non-empty check
used in _SignAllTransactions and SignMessage to maintain consistent behavior.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: cc1656f8-9670-46d9-8014-92233b1986c2

📥 Commits

Reviewing files that changed from the base of the PR and between ffb32de and 0926cc2.

📒 Files selected for processing (2)
  • Runtime/codebase/SolanaMobileStack/MobileWalletAdapterClient.cs
  • Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs

Comment on lines +138 to +144
_authToken = authorization.AuthToken;
var publicKey = new PublicKey(authorization.PublicKey);
if (_walletOptions.keepConnectionAlive)
{
PlayerPrefs.SetString("pk", publicKey.ToString());
PlayerPrefs.SetString(PrefKeyPublicKey, publicKey.ToString());
PlayerPrefs.SetString(PrefKeyAuthToken, _authToken);
PlayerPrefs.Save();
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Minor: empty-AuthToken guard missing on the fresh Authorize() path for consistency.

The commit message states _authToken should be assigned and persisted only when authorization.AuthToken is non-empty, and the equivalent blocks in _SignAllTransactions (lines 204–212) and SignMessage (lines 366–374) already enforce this. The fresh Authorize() path here in _Login still writes unconditionally, which on a spec-nonconforming wallet response would store an empty string to PrefKeyAuthToken (a key just deleted at line 105) and leave _authToken = "" instead of surfacing the failure. Functionally recoverable (next signing call would fall back to Authorize), but inconsistent with the documented guard pattern.

🛡️ Proposed guard for consistency
-            _authToken = authorization.AuthToken;
             var publicKey = new PublicKey(authorization.PublicKey);
-            if (_walletOptions.keepConnectionAlive)
+            if (!string.IsNullOrEmpty(authorization.AuthToken))
             {
-                PlayerPrefs.SetString(PrefKeyPublicKey, publicKey.ToString());
-                PlayerPrefs.SetString(PrefKeyAuthToken, _authToken);
-                PlayerPrefs.Save();
+                _authToken = authorization.AuthToken;
+                if (_walletOptions.keepConnectionAlive)
+                {
+                    PlayerPrefs.SetString(PrefKeyPublicKey, publicKey.ToString());
+                    PlayerPrefs.SetString(PrefKeyAuthToken, _authToken);
+                    PlayerPrefs.Save();
+                }
             }
+            else if (_walletOptions.keepConnectionAlive)
+            {
+                // Persist the public key even if the wallet returned no auth token
+                PlayerPrefs.SetString(PrefKeyPublicKey, publicKey.ToString());
+                PlayerPrefs.Save();
+            }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs` around lines
138 - 144, In _Login where you handle the fresh Authorize() response, avoid
unconditionally assigning and persisting an empty auth token: only set
_authToken and call PlayerPrefs.SetString(PrefKeyAuthToken, ...) when
authorization.AuthToken is non-empty; continue to persist the public key
(publicKey.ToString()) if _walletOptions.keepConnectionAlive is true, but wrap
the _authToken assignment and PrefKeyAuthToken write in the same non-empty check
used in _SignAllTransactions and SignMessage to maintain consistent behavior.

Comment thread Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
Comment thread Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
Only assign _authToken when authorization.AuthToken is non-empty and only persist PlayerPrefs when keepConnectionAlive is true and a valid auth token exists. This prevents overwriting the in-memory token with an empty value and avoids saving empty auth/public key entries to PlayerPrefs.
@JoshhSandhu
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 16, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs`:
- Around line 28-29: The new PlayerPrefs keys PrefKeyPublicKey and
PrefKeyAuthToken silently break existing installs because legacy "pk" and
"authToken" entries stay orphaned; add a one-shot migration in the
SolanaMobileWalletAdapter constructor that checks for the legacy keys ("pk" and
"authToken"), and if present copies their values into PrefKeyPublicKey and
PrefKeyAuthToken (using PlayerPrefs.SetString), deletes the legacy entries
(PlayerPrefs.DeleteKey), and calls PlayerPrefs.Save so existing cached sessions
remain usable after upgrade; reference the class SolanaMobileWalletAdapter, its
constructor, and the constants PrefKeyPublicKey / PrefKeyAuthToken when
implementing.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1b51a68f-9a17-45fd-9658-373ead0d8637

📥 Commits

Reviewing files that changed from the base of the PR and between 0926cc2 and f2f08e5.

📒 Files selected for processing (1)
  • Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs

Comment thread Runtime/codebase/SolanaMobileStack/SolanaMobileWalletAdapter.cs
Added migration of old PlayerPrefs keys ('pk' and 'authToken') to the new PrefKeyPublicKey and PrefKeyAuthToken during SolanaMobileWalletAdapter initialization on Android. The migration copies values only if the new keys are not already set, then deletes the legacy keys and saves PlayerPrefs to preserve user data across updates.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant